Conversation
Only tweak was `unique` having a default, but it isn't used by `pyarrow`, `pandas` - only `polars`
Was a bit more involved since - 4 rhs bin ops and `is_between` aren't defined at this level - polars was missing even more
Eventually we can just require the two `_with_binary*` have defs and then avoid repeating each dunder in every backend
FBruzzesi
left a comment
FBruzzesi
left a comment
There was a problem hiding this comment.
Thanks @dangotbanned this looks promising 🎉 I left a couple of minor comments
| def is_between( | ||
| self, lower_bound: Self, upper_bound: Self, closed: ClosedInterval | ||
| ) -> Self: | ||
| if closed == "left": | ||
| return (self >= lower_bound) & (self < upper_bound) | ||
| if closed == "right": | ||
| return (self > lower_bound) & (self <= upper_bound) | ||
| if closed == "none": | ||
| return (self > lower_bound) & (self < upper_bound) | ||
| return (self >= lower_bound) & (self <= upper_bound) | ||
|
|
||
| def is_duplicated(self) -> Self: | ||
| return ~self.is_unique() |
There was a problem hiding this comment.
There was a problem hiding this comment.
We could fix Series.rename + Series.shape in the same way
There was a problem hiding this comment.
I think it would be a really nice side effect. I know we are not too invested at the moment with the api completeness, but these changes are low hanging fruit, and might simplify a lot the logic there (api completeness) without the need of handling additional special cases
There was a problem hiding this comment.
We could fix
Series.rename+Series.shapein the same way
Thinking back, I'm not super concerned about these two for now
FBruzzesi
left a comment
FBruzzesi
left a comment
There was a problem hiding this comment.
Thanks @dangotbanned - I am pretty much onboarded with these changes. I will wait for @MarcoGorelli approval on this one though!
Just for context, are you considering this PR a requirement for #2962 or viceversa or neither 😂?
| def is_between( | ||
| self, lower_bound: Self, upper_bound: Self, closed: ClosedInterval | ||
| ) -> Self: | ||
| if closed == "left": | ||
| return (self >= lower_bound) & (self < upper_bound) | ||
| if closed == "right": | ||
| return (self > lower_bound) & (self <= upper_bound) | ||
| if closed == "none": | ||
| return (self > lower_bound) & (self < upper_bound) | ||
| return (self >= lower_bound) & (self <= upper_bound) | ||
|
|
||
| def is_duplicated(self) -> Self: | ||
| return ~self.is_unique() |
There was a problem hiding this comment.
I think it would be a really nice side effect. I know we are not too invested at the moment with the api completeness, but these changes are low hanging fruit, and might simplify a lot the logic there (api completeness) without the need of handling additional special cases
Now that #2962 has merged, this part of the plan is possible (#2962 (comment))
Well we've done it this way round now regardless 😄 |
from a look this seems good, i'm running out of time for the day but if you're happy with it feel free to ship it, thanks both! |
FBruzzesi
left a comment
FBruzzesi
left a comment
There was a problem hiding this comment.
Thanks again for this refactor @dangotbanned 🙏🏼
3 participants
What type of PR is this? (check all applicable)
Related issues
{Expr,Series}.is_close#2962{Expr,Series}.is_close#2962 (comment){Expr,Series}.is_close#2962 (comment)Checklist
If you have comments or can explain your changes, please do so below
Creates a common parent protocol for
Compliant{Expr,Series}, spec-ing their shared partsThe diff isn't huge here, but
it'll shrinkit shrinksis_closeand allow us to implement more features in a similar way